fix(coding-agent): recover required compaction loops - #679
Conversation
|
@code-yeongyu All CI checks are green. I do not have upstream merge permission; please merge this PR when ready. |
code-yeongyu
left a comment
There was a problem hiding this comment.
[sisyphus-bot] Brutal review of PR #679. CI is green and the headline behaviors are pinned, but five independent review lanes plus lead-owned reproduction found concrete, reproducible defects that must be fixed before merge. Requesting changes.
Blocker 1 (P1, runtime): successful required-compaction recovery still rejects the originating prompt
packages/coding-agent/src/core/agent-session.ts:1004-1009stores_requiredCompactionAdmissionErrorwhen a queued continuation hits required compaction, but nothing clears it when the post-agent_endrecovery is ACCEPTED and the queued continuation completes successfully._promptAgent()(:1266-1271) then throws the stale error.- Deterministic repro (2/2 runs, faux provider; steer queued mid-turn, inline compaction rejected once, recovery accepted): the queued continuation is persisted ("queued continuation completed", provider calls = 2, both queues empty) yet the originating
session.prompt()still rejects with "Context remains above the compaction threshold because compaction did not complete". - Requested change: clear the stored admission error when the recovery compaction is accepted and its queued continuation is scheduled/launched, plus a regression test at the
post-compaction-recovery-boundsseam that queues the steer BEFORE the failed inline admission.
Blocker 2 (resource safety): deterministic fallback eagerly serializes retained messages unboundedly and executes accessors
packages/coding-agent/src/core/extensions/builtin/compaction/deterministic-fallback.ts:45-55(estimateConservativeTokens) materializesJSON.stringify(message)for every retained message with no size bound and no abort observation. Measured: one 32 MiB retained text message adds ~207 MiB peak memory, synchronously, exactly during required recovery.- The same path invokes property getters on retained tool-call
arguments(probe observedgetterCalls: 2from a crafted arguments object): persisted, attacker-influenceable data is executed-on-read during recovery sizing. - Requested change: bound the estimator with early exit once the
contextWindow - reserveTokensbudget is exceeded, byte-count strings without whole-message serialization, and reject accessor-bearing/non-plain values instead of reading them.
Blocker 3 (test integrity): shipped guards are not pinned
- G1: moving the
turn_endreset out offinally(skipping it on the lane-policy / degradation-recovery early returns) leaves the whole changed suite GREEN (48 files / 335 tests). The exact regression class this PR fixes is unprotected. Add a route-guards case that trips degradation recovery, firesturn_end, and asserts the next turn still admits compaction. - G2: deleting the
agent_startprovenance clear (agent-session.ts:1593-1595) leaves coverage GREEN; a same-text provider error in a later run can inherit stale required-compaction provenance. Add a guards test: complete one real required-compaction run, then feed the identical provider error text in a fresh run and assert no spurious retry/compaction. - C1:
continues once after recovery clears the thresholdqueues only two faux responses while asserting three calls, so the final turn ends in an unassertedNo more faux responses queuederror. Queue a third response and assert its content.
Lane verdicts
- behavior (deep): P1 blocker 1, reproduced.
- safety (deep): blocker 2, measured (~207 MiB / 32 MiB input; accessor execution).
- test review: blockers G1/G2/C1, with sensitivity proofs that the reset, hard-cap order, and provenance gates ARE load-bearing (each deletion goes RED in the shipped tests).
- quality: PASS, no criterion-blocking findings.
- runtime QA lane: BLOCKED by harness-write failure, no runtime evidence; lead will run the isolated real-CLI QA after fixes.
Clean-worktree receipt at review time: git status --porcelain empty in /Users/yeongyu/local-workspaces/senpi/local-ignore/worktrees/pr-679-brutal-review. Evidence: local-ignore/qa-evidence/20260803-pr-679-brutal-review/ (reviewer reports + repro scripts).
|
[sisyphus-bot] Review follow-up: all verified blockers from my request-changes review are now fixed and verified. Since I cannot push to the contributor fork, the fixes landed as a stacked branch
One review item was declined after lead verification: the stale-provenance concern (G2) could not be reproduced as harmful — classification at agent_end also requires a still-over-threshold context, which the successful run-2 compaction eliminates; the mutation stayed GREEN under every formulation. Evidence: 28/28 changed-scope tests, typecheck, |
…p-review-fixes fix(coding-agent): required-compaction review fixes for #679
Summary
Verification
npm run checkorigin/mainnpm run buildSummary by cubic
Fix required-compaction recovery so long sessions keep progressing and interrupted work resumes correctly. Resets the soft compaction cap per provider turn and adds safe, deterministic suffix recovery even without usage anchors.
Written for commit e863482. Summary will update on new commits.